Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve CMake behavior on Unix and macOS #66369

Merged
merged 3 commits into from
Jun 25, 2023

Conversation

perryprog
Copy link
Contributor

@perryprog perryprog commented Jun 21, 2023

Summary

None

Purpose of change

I noticed a few build and install issues popping up after #65822 for me on macOS. Namely:

  1. I was getting a linker error on compilation due to CoreFoundation being used but not linked in CMake
  2. Installation locations were hecked up when using -DUSE_PREFIX_DATA_DIR=Yes because... okay, I'm not sure why but stuff would be installed to /usr/local instead of within /usr/local/share/cataclysm-dda as you might expect.

Describe the solution

  1. Add CoreFoundation as a framework when building on macOS.
  2. Set CMAKE_INSTALL_DATAROOTDIR to include the /share/cataclysm-dda suffix when building with USE_PREFIX_DATA_DIR. I'm uncertain if this is the ideal solution—@alef, do you know?
    • I also wonder if USE_PREFIX_DATA_DIR should just be on by default on Unix since I'm pretty sure this is what most Unix people would want, but I'm content to leave it as is for now.

Describe alternatives you've considered

Testing

Stuff builds and installs in the right location. I'm hoping CI will catch any other obvious issues...

Additional context

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 21, 2023
@perryprog
Copy link
Contributor Author

Going to try making USE_PREFIX_DATA_DIR the default to see if that makes CI happy. (And because it's probably better for Unix users.)

I'm not sure when exactly this became a problem, but msgfmt expects its output
directory to exist, which includes LC_MESSAGES and CMake wasn't doing that. This
should also correct a CI failure for CMake.

My guess is this issue went unnoticed for a bit since if the LC_MESSAGES
directory was previously created (e.g., by running lang/update_pot.sh) it tends
to stay around until you do a fresh clone of the repo.
@github-actions github-actions bot added the Translation I18n label Jun 22, 2023
@perryprog
Copy link
Contributor Author

I'm confident enough that this should be good to merge despite the flaked CI failure stopping the CMake job from actually running.

@andrei8l
Copy link
Contributor

I'm confident enough that this should be good to merge despite the flaked CI failure stopping the CMake job from actually running.

The CMake job is already broken from the last CMake PR. Let's make certain it works this time

@perryprog
Copy link
Contributor Author

The CMake job is already broken from the last CMake PR. Let's make certain it works this time

Yeaaaaah, you're right. My impatience was just getting to me. 🙃

@dseguin dseguin merged commit 9404d56 into CleverRaven:master Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Build Issues regarding different builds and build environments json-styled JSON lint passed, label assigned by github actions Translation I18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants